Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format SQL in watcher #706

Merged
merged 8 commits into from
Aug 29, 2019
Merged

Conversation

barryvdh
Copy link
Contributor

This formats the SQL in PHP (instead of Javascript) so that they can be shown in the preview window (imho that makes it a lot more clear when looking at them, but might be preference, hence the config options).
It stores the familyHash to be able to check for duplicates (which isn't implemented anywhere yet, but would perhaps be useful).

The view of the Javascript should be the same, with/without pre-formatted SQL.

@barryvdh
Copy link
Contributor Author

barryvdh commented Aug 22, 2019

Added summary of the queries (total, duplicated and total duration):
Screen Shot 2019-08-22 at 21 55 41

(Assets need to be rebuild after merging)

@taylorotwell
Copy link
Member

It's a little unclear to me what the "format_sql" configuration option does. I think it will also be unclear to end users.

@barryvdh
Copy link
Contributor Author

So how would you call it? Or would you do it as default?

@barryvdh
Copy link
Contributor Author

It's replacing the ? parameters with the actual bindings, so you can see the query as if it was prepared. So instead of id = ? you get id = 3, so basically what the javascript was doing now on details, but now directly.

@taylorotwell
Copy link
Member

Why make it a configuration option at all? Why would someone want to disable it?

@barryvdh
Copy link
Contributor Author

Not sure, because it wasn't done before I thought maybe that was on purpose.

@barryvdh
Copy link
Contributor Author

I've removed the option, just always do it now.

@barryvdh
Copy link
Contributor Author

Fixed+added a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants